feat: use nh3 for HTML sanitization by nouralmaa · Pull Request #10264 · ietf-tools/datatracker

nouralmaa · 2026-01-15T17:14:56Z

nouralmaa · 2026-01-15T17:20:19Z

ietf/utils/html.py

+# Allow the protocols/tags/attributes we specifically want, plus anything that nh3 declares
+# to be safe.
+
+acceptable_protocols = nh3.ALLOWED_URL_SCHEMES.union(


TODO check. this is more permissive than bleach's strict superset. however, unsure if this adds extra vulnerabilities since nh3 uses a better maintained parser https://github.com/rust-ammonia/ammonia

@jennifer-richards @rjsparks any thoughts?

I'm going to try to get a few other eyes on this

I think adding tel: and ftp: (really?) are fine from a security perspective.

We don't really need ftp, but tel might be useful.

codecov · 2026-01-15T17:45:19Z

Codecov Report

❌ Patch coverage is 91.66667% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.36%. Comparing base (4945809) to head (3d66b11).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
ietf/utils/markdown.py	89.36%	5 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #10264   +/-   ##
=======================================
  Coverage   88.36%   88.36%           
=======================================
  Files         325      325           
  Lines       43653    43652    -1     
=======================================
  Hits        38573    38573           
+ Misses       5080     5079    -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

martinthomson · 2026-01-18T23:29:33Z

ietf/utils/html.py

-        "li", "ol", "p", "pre", "q", "s", "samp", "small", "span", "strike", "style",
+        "li", "ol", "p", "pre", "q", "s", "samp", "small", "span", "strike",
        "strong", "sub", "sup", "table", "title", "tbody", "td", "tfoot", "th", "thead",
        "tr", "tt", "u", "ul", "var"


xmp is also good

Suggested change

"tr", "tt", "u", "ul", "var"

"tr", "tt", "u", "ul", "var", "xmp"

martinthomson · 2026-01-18T23:31:32Z

ietf/utils/html.py

+# to be safe.
+
+acceptable_protocols = nh3.ALLOWED_URL_SCHEMES.union(
    {"http", "https", "mailto", "ftp", "xmpp"}


Suggested change

{"http", "https", "mailto", "ftp", "xmpp"}

{"ftp", "http", "https", "mailto", "tel", "xmpp"}

Sort. Add "tel".

(If performance is dictated by order, move "https" to the top.)

martinthomson · 2026-01-18T23:33:45Z

ietf/utils/html.py

-    protocols=acceptable_protocols,
-    strip=True,
+    url_schemes=acceptable_protocols,
+    link_rel=None


This doesn't seem right to me. The default value is safer. Are there cases where links opened will need window.opener? I can't imagine that being necessary for user-generated content.

Same below.

cool, changed it here as there were no rel attributes before

martinthomson · 2026-01-18T23:34:58Z

ietf/utils/html.py

-    protocols=acceptable_protocols,
-    strip=True,
+_liberal_nh3_cleaner = nh3.Cleaner(
+    tags=acceptable_tags.union({"mg", "figure", "figcaption"}),


typo?

Suggested change

tags=acceptable_tags.union({"mg", "figure", "figcaption"}),

tags=acceptable_tags.union({"img", "figure", "figcaption"}),

martinthomson · 2026-01-18T23:35:51Z

ietf/utils/html.py

    """Returns the given HTML sanitized, and with the given tags removed."""
    allowed = acceptable_tags - set(t.lower() for t in tags)
-    return bleach.clean(html, tags=allowed, strip=True)
+    return nh3.clean(html, tags=allowed)


You might want to audit invocations of this function.

Not sure if @martinthomson did so himself, but it looks to me like the only use is via the removetags filter in htmlfilters.py, which is not used anywhere (I also checked the DBTemplates)

If so, we can just lose this method entirely.

If not, I think this changes remove_... to escape_... because nh3 doesn't have a strip option (but that's just based on reading docs)

Doesn't seem to raise any validation errors

rjsparks · 2026-01-30T18:57:50Z

Hi @nouralmaa - Thanks for continuing to work on this.

One point on something that's not obvious - commits bf72278 and 9736b74 touch an old data migration (see the filename) - those will never run again, and its better to not touch these migration files as they should show what happened when they actually ran on the production database. If it's easy, please revert those commits. If it's not, we'll do it when this gets towards final review.

When we have major changes to django (we may have one coming with django5) we tend to squash the migrations and that one would go away at that time.

nouralmaa · 2026-01-30T19:15:49Z

ietf/utils/templatetags/tests.py

            '<a href="https://www.ietf.org" rel="nofollow">https://www.ietf.org</a>',
        )
+        self.assertEqual(
+            linkify("https://mailman3.ietf.org/mailman3/lists/[email protected]/"),


fixes #10120

jennifer-richards · 2026-02-05T15:13:51Z

ietf/utils/templatetags/textfilters.py

-        value = conditional_escape(value)
-    text = mark_safe(_linkify(value))  # _linkify is a safe operation
+        value = urlize(value, autoescape=True)  # _linkify is a safe operation
+    text = mark_safe(value)


This won't work as written - it needs to call urlize() in all cases, not only when autoescape is true. However,

The commit log around ietf.doc.templatetags.ietf_filters.urlize mentions issues with Django's urlize and its handling of adjacent parentheses. Need to investigate that and whether it's been fixed.

If it is ok to switch to urlize, we should just do away with linkify as it's redundant.

have adjusted so this can be called in both cases.

re the handling of adjacent parentheses and this commit, I can understand why django's urlizer might not be ideal for linkification like [REF](http://example.com/foo), and the issue still stands. however, I think the markdown filter inside ietf.utils.templatetags.htmlfilters can be used in such cases with links intact

yes, but it would require changing lots of internal html templates and tests to do away with the linkify filter completely. maybe can be raised in a separate issue? can also try to have a look soon.

jennifer-richards · 2026-02-05T15:17:07Z

ietf/group/tests_info.py


    def test_view_status_update(self):
        chair = RoleFactory(name_id='chair',group__type_id='wg')
-        event = GroupEventFactory(type='status_update',group=chair.group)


Unexpected change. Unless there's a reason not to test this behavior, need to fix the assertion below rather than drop the test.

jennifer-richards · 2026-02-05T15:51:45Z

ietf/utils/html.py

    """Returns the given HTML sanitized, and with the given tags removed."""
    allowed = acceptable_tags - set(t.lower() for t in tags)
-    return bleach.clean(html, tags=allowed, strip=True)
+    return nh3.clean(html, tags=allowed)


Not sure if @martinthomson did so himself, but it looks to me like the only use is via the removetags filter in htmlfilters.py, which is not used anywhere (I also checked the DBTemplates)

If so, we can just lose this method entirely.

If not, I think this changes remove_... to escape_... because nh3 doesn't have a strip option (but that's just based on reading docs)

nouralmaa · 2026-02-09T13:53:40Z

Hi @nouralmaa - Thanks for continuing to work on this.

One point on something that's not obvious - commits bf72278 and 9736b74 touch an old data migration (see the filename) - those will never run again, and its better to not touch these migration files as they should show what happened when they actually ran on the production database. If it's easy, please revert those commits. If it's not, we'll do it when this gets towards final review.

When we have major changes to django (we may have one coming with django5) we tend to squash the migrations and that one would go away at that time.

re these commits, I've reverted the changes and also made a change to use html instead of having the links hardcoded into the slugs for that template. I imagine that may be necessary down the road in any case, but feel free to revert if not.

nouralmaa added 2 commits January 15, 2026 16:41

feat: use nh3 for html sanitizing

0330e40

chore: comments

806448b

nouralmaa commented Jan 15, 2026

View reviewed changes

chore: stray whitespace

91e3410

martinthomson reviewed Jan 18, 2026

View reviewed changes

nouralmaa added 4 commits January 19, 2026 15:52

chore: add tel protocol and xmp tag

bb4b6d3

chore: fix typo

2c7704b

tests: add tag removal tests

a0665ac

feat: rel noopener noreferrer

356cedb

This comment was marked as spam.

Sign in to view

nouralmaa added 11 commits January 26, 2026 21:27

feat: markdown linker extention

6042400

chore: refractor and add email validation

1aeb662

chore: add linkable protocols

31c4153

chore: remove bleach

9f0a249

fix: don't use default nh3 protocols

4136809

refractor: use django's urilizer inside templates

c39659b

fix: adjust template linker

f2de311

tests: remove bleach tests

153b742

fix: templat fixes

90c47a7

chore: code cleanup

fedd691

fix: adjust migration

bf72278

nouralmaa force-pushed the replace-bleach-nh3 branch from 821b2a9 to bf72278 Compare January 29, 2026 21:22

nouralmaa added 2 commits January 29, 2026 21:27

chore: iesg email bracket

9736b74

fix: adjust migration

cbbd096

tests: add mailman tests

9cc2773

nouralmaa commented Jan 30, 2026

View reviewed changes

revert: revert migration commit

426cbaa

nouralmaa marked this pull request as ready for review January 30, 2026 19:37

jennifer-richards reviewed Feb 5, 2026

View reviewed changes

nouralmaa added 6 commits February 9, 2026 13:23

chore: lose removetags filter

50b2814

fix: adjust group test

fe5ecd5

fix: adjust urlize autoescaping

ef8ee50

tests: add markdown linker tests

6bf39ba

tests: add sanitizer tests

35493f4

refractor: use template for important dates

731c907

nouralmaa added 2 commits February 13, 2026 15:17

chore: lose linkify

2d8fae0

Merge branch 'main' into replace-bleach-nh3

3d66b11

	"tr", "tt", "u", "ul", "var"
	"tr", "tt", "u", "ul", "var", "xmp"

	{"http", "https", "mailto", "ftp", "xmpp"}
	{"ftp", "http", "https", "mailto", "tel", "xmpp"}

	tags=acceptable_tags.union({"mg", "figure", "figcaption"}),
	tags=acceptable_tags.union({"img", "figure", "figcaption"}),

Conversation

nouralmaa commented Jan 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jan 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

rjsparks commented Jan 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nouralmaa commented Feb 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nouralmaa commented Jan 15, 2026 •

edited

Loading

codecov bot commented Jan 15, 2026 •

edited

Loading

rjsparks commented Jan 30, 2026 •

edited

Loading